erofs: Fix reading directory entries #188
Conversation
| } | ||
|
|
||
| if inode.mode().is_dir() { | ||
| if !inode.inline().is_empty() { |
There was a problem hiding this comment.
Did you have a reason to change the traversal ordering too so the tail block is traversed first?
There was a problem hiding this comment.
The ordering was the root cause as I'd initially thought
| this.visit_nid(&img, nid)?; | ||
| } | ||
| Ok(this.objects) | ||
| } |
There was a problem hiding this comment.
FTR I think it'd be relatively straightforward to generate unit tests for all of this code.
…ry bug Add a comprehensive regression test that reproduces the bug fixed in PR inline directory sections as DirectoryBlock, causing a panic. The test generates an erofs image on-the-fly using C mkcomposefs, which (unlike the Rust implementation) creates directories with empty inline sections. This reveals a key difference: the C implementation stores only the parent reference without . and .. entries when a directory is empty, while the Rust implementation always includes those entries. The test demonstrates both: 1. A synthetic case showing DirectoryBlock::n_entries() panics on empty inline data 2. The actual bug triggered by collect_objects() on the C-generated image The test is marked #[ignore] so it doesn't break CI until PR containers#188's fix is applied. When run with --ignored, it passes by catching the expected panics. Asissted-by: Claude Code Signed-off-by: Colin Walters <[email protected]>
|
Can we land #189 and then this can be rebased on top ? |
An empty directory inode is actually an invalid directory. Before the |
Read inline inode entries only if not empty Before this patch, gathering objects from EROFS, the program panics with the following error ``` thread 'main' panicked at crates/composefs/src/erofs/reader.rs:404:13: range end index 12 out of range for slice of length 0 ``` Signed-off-by: Pragyan Poudyal <[email protected]>
Johan-Liebert1
force-pushed
the
erofs-dir
branch
from
8cd1b65 to
8141706
Compare
|
So, apparently the issue was not with the reading order, but parsing inline entry even when they were empty |
Doing `x % y` is universally understood IMO Signed-off-by: Pragyan Poudyal <[email protected]>
Johan-Liebert1
force-pushed
the
erofs-dir
branch
from
d0a8d9f to
ee21853
Compare
| //! images, including inode traversal, directory reading, and object | ||
| //! reference collection for garbage collection. | ||
| #![allow(clippy::manual_is_multiple_of)] |
There was a problem hiding this comment.
Not going to disagree, but OTOH isn't it easier to follow what clippy wants and use is_multiple_of() ?
The bigger picture thing here is that I think it's too painful to gate on all clippy lints by default, in bootc we do this https://github.com/bootc-dev/bootc/blob/7e526508a9e7119cd8f2157936f31771da9a42df/Makefile#L99-L104
Because otherwise, exactly this will happen periodically - a new rustc release will come out and likely kill CI for minor style lints.
Thanks for commenting. Is this not something that |
4 participants
Read inline inode entries only if not empty
Before this patch, gathering objects from EROFS, the program panics with
the following error